Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add acts_as_list to Spree::StockLocation #2953

Merged

Conversation

rymai
Copy link
Contributor

@rymai rymai commented Nov 14, 2018

Fixes #2952.

/cc @octave

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, is the position of StockLocations used anywhere in core currently?

@rymai
Copy link
Contributor Author

rymai commented Nov 15, 2018

Actually I realized that it doesn't look so... I bumped into this issue when trying to reorder the stock locations in the Admin (the UI still allows to do that, so maybe we should just remove that...).

That also triggers a more interesting discussion: my original thought was that the stock locations ordering would be used in the simple shipment coordinator, but it's not. At least I think we should change @stock_locations = Spree::StockLocation.active to @stock_locations = Spree::StockLocation.order_default.active there... but that's another discussion. :D

@kennyadsl
Copy link
Member

kennyadsl commented Nov 15, 2018

That also triggers a more interesting discussion: my original thought was that the stock locations ordering would be used in the simple shipment coordinator, but it's not. At least I think we should change @stock_locations = Spree::StockLocation.active to @stock_locations = Spree::StockLocation.order_default.active there... but that's another discussion. :D

I think you want to look at this recently merged PR #2783 that would allow to use the position if needed creating and using a new StockLocation sorter class.

I think we need to merge this PR anyway since there's a bug in the UI. Thanks again!

@rymai
Copy link
Contributor Author

rymai commented Nov 15, 2018

I think you want to look at this recently merged PR #2783 that would allow to use the position if needed creating and using a new StockLocation sorter class.

@kennyadsl Thanks for the pointer! That will fulfill my need perfectly! Looking forward for the next version! :)

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@tvdeyen
Copy link
Member

tvdeyen commented Nov 16, 2018

@rymai would you mind to rebase with latest master to fix the build issues? Thanks

Fixes solidusio#2952.

Signed-off-by: Rémy Coutable <remy@rymai.me>
@rymai rymai force-pushed the 2952-add-acts_as_list-to-stock_location branch from d0f2200 to fd22315 Compare November 16, 2018 10:19
@rymai
Copy link
Contributor Author

rymai commented Nov 16, 2018

@tvdeyen Done, thanks! 🎉

@tvdeyen
Copy link
Member

tvdeyen commented Nov 16, 2018

Thank you for your contribution

@kennyadsl kennyadsl merged commit bd75b8c into solidusio:master Nov 20, 2018
@rymai rymai deleted the 2952-add-acts_as_list-to-stock_location branch February 20, 2019 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants